Add comprehensive tests for ExecutionListenerAdapter #5162
Conversation
Resolves the TODO comment by implementing tests for all adapter methods that were previously untested. This significantly improves test coverage of the ExecutionListenerAdapter class. Tests added: - testDynamicTestRegistered(): Verifies dynamic test registration - testExecutionStarted(): Verifies test execution start notification - testExecutionSkipped(): Verifies test skip notification with reason - testExecutionFinished(): Verifies test execution completion - testFileEntryPublished(): Verifies file entry publication The MockTestExecutionListener was extended to capture all callback invocations, enabling proper verification of each adapter method. All tests follow the existing pattern and verify that: 1. The adapter correctly translates TestDescriptor to TestIdentifier 2. All parameters are properly passed through to the listener 3. The TestPlan is correctly consulted for identifier lookup
There was a problem hiding this comment.
Thanks for trying to clean up a TODO!
I suspect the other methods were not tested because they're a) simple enough to verify visually and b) at the time not spy-able with Mockito. This would have made the MockTestExecutionListener rather verbose for what it tests and that is also my critique of the current set of tests.
However, at a glance (I haven't tried this) that Mockito issues has been resolved, so not only would it would be preferable to use Mockito, it would also solve some of the verbosity issues.
Further more, the PR template includes affirmation that you agree to the JUnit Contributor License Agreement and a Definition of Done. It appears you've omitted these from your PR description. Please update your PR description to include these. You can find the text of the template in github/PULL_REQUEST_TEMPLATE.md.
| var testPlan = InternalTestPlan.from(discoveryResult); | ||
|
|
||
| var testExecutionListener = new MockTestExecutionListener(); | ||
| var executionListenerAdapter = new ExecutionListenerAdapter(testPlan, testExecutionListener); |
There was a problem hiding this comment.
The lines above are identical in all other test methods. Please extract these to a single method.
| var testIdentifier = testPlan.getTestIdentifier(testDescriptor.getUniqueId()); | ||
|
|
||
| //not yet spyable with mockito? -> https://github.com/mockito/mockito/issues/146 | ||
| var testExecutionListener = new MockTestExecutionListener(); |
There was a problem hiding this comment.
The chosen solution extends the MockTestExecutionListener, but the issue in the above comment has been resolved.
So presumably it is now possible to mock/spy the TestExecutionListener. Is that correct?
mpkorstanje
added
type: task
theme: reporting
component: Platform
status: waiting-for-feedback
theme: discovery
and removed
theme: reporting
labels
3 participants
Summary
Resolves the TODO comment in
ExecutionListenerAdapterTestsby implementing tests for all adapter methods that were previously untested. This significantly improves test coverage of theExecutionListenerAdapterclass.Motivation
The test file had a TODO comment:
// TODO Test other adapter methods.Only 1 out of 6 methods was tested (
reportingEntryPublished), leaving 83% of the adapter's functionality untested.Changes
Added comprehensive tests for all previously untested methods:
testDynamicTestRegistered()- Verifies dynamic test registration handlingtestExecutionStarted()- Verifies test execution start notificationstestExecutionSkipped()- Verifies test skip notifications with reasonstestExecutionFinished()- Verifies test execution completion handlingtestFileEntryPublished()- Verifies file entry publicationImplementation Details
MockTestExecutionListenerto capture all callback invocationstestReportingEntryPublishedTestDescriptortoTestIdentifierTestPlanis correctly consulted for identifier lookupTesting
All new tests pass and follow the established patterns in the codebase.
Resolves TODO at line 33 in
ExecutionListenerAdapterTests.java